refactor(backup): remove duplication, unused signals and Waku naming #6967
Conversation
Jenkins BuildsClick to see older builds (24)
|
|
This is a breaking change since I removed some signals, but they were not used anymore, and if they were, they weren't handled, just caught. Desktop PR: status-im/status-desktop#18926 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (33.51%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #6967 +/- ##
===========================================
+ Coverage 55.34% 59.52% +4.18%
===========================================
Files 833 834 +1
Lines 120215 120103 -112
===========================================
+ Hits 66529 71490 +4961
+ Misses 46706 41336 -5370
- Partials 6980 7277 +297
Flags with carried forward coverage won't be shown. Click here to find out more.
|
jrainville
force-pushed
the
refactor/backup-cleanup-and-deduplication
branch
from
16f8892 to
030a26f
Compare
There was a problem hiding this comment.
I moved this file and removed FetchingDataProgress
| } | ||
|
|
||
| // TODO use a new feed for accounts service events | ||
| s.messenger.PublishSettingEvent(settingField) |
There was a problem hiding this comment.
This is the part where I couldn't get rid of the messenger. I didn't manage to make another event feed work
node/status_node_services.go
Outdated
| b.config, | ||
| b.accountsPublisher, | ||
| mediaServer, | ||
| b.logger.Named("accountsService"), |
There was a problem hiding this comment.
| b.logger.Named("accountsService"), | |
| b.logger.Named("AccountsService"), |
There was a problem hiding this comment.
It's good to have a common place and don't duplicate the code. But ideally this shouldn't be here 😄
Neither Messenger or protocol should be responsible for putting data into the database. Accounts service is the right place for this procedure. So in perfect world, protocol would just pass the message to certain service (Accounts in this case) for handling.
But... this is not doable at the moment, so let's leave it as you did it for now 🤷 Would be great if you can leave a comment mentioning this. When we refactor the protocol/messenger package in proper way, we will address this.
jrainville
force-pushed
the
refactor/backup-cleanup-and-deduplication
branch
2 times, most recently
from
4c5b94f to
b85cac1
Compare
|
@igor-sirotin I'm not sure the code coverage is working well here. I mostly moved or removed code. It seems like it counts some of the removals I made as lowering the coverage, which is weird. Do you think it's ok to force merge? |
jrainville
force-pushed
the
refactor/backup-cleanup-and-deduplication
branch
from
b85cac1 to
45aa32b
Compare
Moves HandleSyncWatchOnlyAccount to the new syncing package where it is shared by the wallet service and the messenger. Removing the messenger dependency and duplication
Fixes status-im/status-desktop#18344 Removes the duplication in the accounts service by moving the shared code to the syncing module. Removes as much as much as possible the use of the Messenger. The signal sending still uses the Messenger for now.
jrainville
force-pushed
the
refactor/backup-cleanup-and-deduplication
branch
from
45aa32b to
2d8673c
Compare
|
force-merging as the code was mostly moved around |
4 participants
Fixes status-im/status-desktop#18344
Removes the duplication that was introduced during the Local backup Epic.
Also removes most of the Messenger access from the Accounts service. I couldn't remove it all, because first it was already used before the local backup was introduced and I still need the Messenger's signal manager. I tried using the WalletFeed and introducing a new Feed, but both of those didn't work.
Finally, this PR removes some no longer used signals and removes the name
Wakufrom the backup signals, since they are no longer related or using Waku.